-
Notifications
You must be signed in to change notification settings - Fork 317
Add copy-to-clipboard buttons in RawLogTable
for log line data and URLs
#1227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 3d26a38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
E2E Test Results✅ All tests passed • 25 passed • 3 skipped • 223s
|
…le for improved accessibility
…ionality; refactor copy buttons in DBRowTable for improved consistency style: adjust button background color for better visibility
…oved code clarity
…ld interaction and copy functionality
…mproved layout consistency
Co-authored-by: Mike Shi <[email protected]>
e668986
to
5dfa32a
Compare
@elizabetdev the issue isn't with exclude specifically, just string rows in general that are json. When I click the filter button on following data for example: This is what the sidebar shows: I do see the same behavior here: so maybe it's indented, thought I'd confirm its intentional / or another ticket/issue |
Thanks @brandon-pereira! I see the issue now. I had assumed the filters would work just by adding them to any field (even though for something like For now, I’ve enabled filters only for Since this PR is mainly about setting up the UI to support field actions, I think it makes sense for an engineer to define the actual logic for showing more actions later on. |
Perfect, that makes sense. This PR LGTM - based on the above- do we want to re-introduce the exclude filter? I can see that being valuable for some. |
…ude and exclude actions
Claude finished @teeohhem's task —— View job Need Clarification
Hi @teeohhem! I see you've mentioned me, but I'm not sure what you'd like me to help with. Here are some things I can do:
Could you please clarify what you'd like me to do? |
Code Review - PR #1227Thank you for this feature addition! This PR adds useful copy-to-clipboard functionality and filtering options to the log table. Here's my detailed review: ✅ StrengthsGood Architecture & Component Separation
UX Improvements
Code Quality
🔍 Issues & Concerns1. Memory Leaks (Critical) 🔴DBRowTableFieldWithPopover.tsx:76-80 & 22-27
Fix Required: useEffect(() => {
return () => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
if (hoverDisableTimeoutRef.current) {
clearTimeout(hoverDisableTimeoutRef.current);
}
};
}, []); Same issue in DBRowTableRowButtons.tsx - the setTimeout callbacks need cleanup. 2. Error Handling Missing 🟡DBRowTableFieldWithPopover.tsx:76-80 & DBRowTableRowButtons.tsx:22-40
Recommendation: const copyFieldValue = async () => {
try {
const value = typeof cellValue === 'string' ? cellValue : `${cellValue}`;
await navigator.clipboard.writeText(value);
setIsCopied(true);
setTimeout(() => setIsCopied(false), 2000);
} catch (error) {
console.error('Failed to copy to clipboard:', error);
// Consider showing an error toast notification
}
}; 3. Accessibility Issues 🟡DBRowTableFieldWithPopover.tsx:137-144
Recommendations: <span
onMouseEnter={handleMouseEnter}
onMouseLeave={handleMouseLeave}
onClick={handleClick}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
handleClick();
}
}}
tabIndex={0}
role="button"
aria-label="Show field actions"
style={{ cursor: 'pointer' }}
> 4. Type Safety 🟡DBRowTableFieldWithPopover.tsx:15 cellValue: any; // ❌ Using 'any' type Recommendation: cellValue: unknown; // Better type safety Then use type guards in the conversion: const value = typeof cellValue === 'string'
? cellValue
: String(cellValue ?? ''); 5. URL Manipulation Security 🟡DBRowTableRowButtons.tsx:29-36
Recommendations:
6. Performance Considerations 🟢DBRowTable.tsx:893-924
Minor Issue:
7. Hardcoded Values 🟢DBRowTableFieldWithPopover.tsx:39-43 const canFilter =
columnName &&
(columnName === 'ServiceName' || columnName === 'SeverityText') &&
... Recommendation: const FILTERABLE_COLUMNS = ['ServiceName', 'SeverityText'];
const canFilter = columnName && FILTERABLE_COLUMNS.includes(columnName) && ... 8. Code Style (Minor) 🟢packages/api/tsconfig.json
🧪 Testing CoverageMissing:
Recommended Tests:
🔒 Security Assessment✅ No obvious security vulnerabilities for malicious code execution 📊 Overall AssessmentCode Quality: 7/10
Feature Implementation: 8/10
Production Readiness: 6/10
🎯 Recommendations Before MergeMust Fix (Blocking):
Should Fix:
Nice to Have:
Great work on the feature implementation! The UX improvements are solid and the component architecture follows good React patterns. The main concerns are the memory leaks and error handling which should be addressed before merging. Let me know if you'd like clarification on any of these points! 🚀 |
98b4122
PR Review: Add copy-to-clipboard buttons in RawLogTableGreat work on this feature addition! The implementation is well-structured and follows the codebase conventions. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues1. Missing Clipboard API FallbackSeverity: Medium The clipboard API might not be available in all browsers or contexts (e.g., non-HTTPS). Consider adding a fallback for older browsers or insecure contexts. 2. Hardcoded Column Names for FilteringSeverity: Low The filterable columns are hardcoded (ServiceName, SeverityText). Consider making this configurable via props or a constant array for better maintainability. 3. Type Safety IssueSeverity: Low Using 'any' type for cellValue goes against the codebase guideline. Consider using a union type like: string | number | boolean | null | undefined ⚡ Performance Considerations
🔒 Security Considerations
🧪 Test CoverageConcern: No test files were added for the new components. Consider adding:
📝 Code Quality Suggestions1. Extract Magic NumbersLocation: DBRowTableFieldWithPopover.tsx:69, 85 Consider extracting timeout values (100ms, 1000ms) to named constants at the top of the file for better maintainability. 2. Inconsistent iconSize UsageLocation: Various files The icon sizes vary (14 vs 12). Consider standardizing or documenting why they differ. 3. Accessibility ImprovementsCurrent: tabIndex={-1} and aria-hidden="true" on the popover target Consider:
🎨 Style & Convention Observations
🔄 API ChangesModified: RowSidePanelContextProps.onPropertyAddClick signature now includes an optional action parameter Impact: This is backward compatible due to the optional parameter. Good design! 👍 🎯 Recommendations
✨ Overall AssessmentThis is a solid feature implementation that follows the codebase conventions well. The code is clean, well-structured, and demonstrates good React patterns. With the addition of tests and minor improvements suggested above, this will be production-ready. Recommendation: Approve with minor suggestions for improvement. Great work, @elizabetdev! 🎉 |
Code Review - PR #1227Thanks for the contribution! This PR adds useful copy-to-clipboard functionality for log table rows and fields. Overall, the implementation is solid and follows good practices. Here are my findings: ✅ Strengths
🐛 Potential Bugs
|
// Parse JSON string fields to make them proper JSON objects | ||
const parsedRow = Object.entries(cleanRow).reduce( | ||
(acc, [key, value]) => { | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a ticket.
Closes HDX-2436 and closes HDX-2534
This PR adds copy-to-clipboard buttons in
RawLogTable
for log line data and URLs, with improved button styling. It also adds row field buttons.